Add degenerate-shape test coverage for D8 hydro functions (#2713)#2720
Merged
brendancol merged 6 commits intoJun 1, 2026
Merged
Conversation
…trib#2713) Adds 1x1, 1xN, and Nx1 regression tests for flow_accumulation_d8, flow_direction_d8, flow_length_d8, and hand_d8. These functions already handle single-pixel and strip inputs; the tests lock in that behavior so a regression in the kernel-boundary path would be caught. Test-only, no source changes.
brendancol
commented
May 29, 2026
Contributor
Author
brendancol
left a comment
brendancol
left a comment
There was a problem hiding this comment.
PR Review: Add degenerate-shape test coverage for D8 hydro functions (#2713)
Test-only change. Adds 1x1, 1xN, and Nx1 cases to four D8 test files. I read all four diffs against the source functions.
Blockers
None.
Suggestions
test_hand_d8.pytest_degenerate_shape: this one only checks output shape. With fa=200 and threshold=100, every cell is a stream, so HAND should be 0 everywhere. Asserting that value would catch a regression that returns the wrong magnitude, not just the wrong shape. The other three tests check a value-level property (all-NaN, min >= 1, no NaN), so this is the odd one out.
Nits
- All four tests use the same name (
test_degenerate_shape) with near-identical bodies. Fine across separate files. If they ever get pulled into a shared parametrized helper, the per-function edge semantics (all-NaN vs finite) would need to stay distinct.
What looks good
- Assertions match the current behavior for each function. I verified: flow_direction returns all-NaN because every cell is an edge cell, and the other three stay finite.
- flow_length covers both direction modes.
- numpy-only scope makes sense for a kernel-boundary check, and the PR body says so. Cross-backend parity is already covered by existing tests in each file.
- Reuses the existing builders (
_make_flow_dir_raster,_make_hand_rasters,create_test_raster), no new helpers.
Checklist
- Assertions match verified current behavior
- NaN handling correct (flow_direction all-NaN edge case)
- Edge cases covered (this PR is the edge-case coverage)
- [n/a] Dask chunk boundaries (numpy-only by design)
- No premature materialization
- [n/a] Benchmark (test-only)
- [n/a] README matrix (no API change)
- Docstrings present on each test
brendancol
commented
May 29, 2026
Contributor
Author
brendancol
left a comment
brendancol
left a comment
There was a problem hiding this comment.
Follow-up review (#2713)
Addressed the one Suggestion from the first pass.
- Fixed:
test_hand_d8.pytest_degenerate_shapenow assertsHAND == 0for the all-stream degenerate input, not just the output shape. Verified the value before adding the assert; the strengthened test passes. - Dismissed (with reason): the Nit about the shared
test_degenerate_shapename. The four tests live in separate files and keep distinct per-function semantics (all-NaN for flow_direction, finite/zero for the others). No shared helper is planned, so the duplicate name is not a problem here.
No other findings remain.
Contributor
Author
PR Review: Add degenerate-shape test coverage for D8 hydro functions (#2713)Test-only PR adding 1x1, 1xN, and Nx1 regression tests to four D8 hydrology functions. I read all four changed test files in full and ran the 15 new cases on a CUDA host: all 15 passed. The hand and flow_direction tests assert the exact output. The other two assert weaker conditions than the value they document. Blockers (must fix before merge)None. Suggestions (should fix, not blocking)
Nits (optional improvements)None. What looks good
Checklist
|
flow_accumulation and flow_length degenerate-shape tests asserted only min >= 1.0 and not-NaN. The exact output is 1.0 and 0.0 everywhere, so the loose checks would pass through a regression that returned a wrong finite value. Assert the exact arrays instead.
Contributor
Author
|
Followup da3af03: applied both suggestions. flow_accumulation now asserts the result equals 1.0 exactly and flow_length asserts 0.0 exactly, instead of the looser min >= 1.0 / not-NaN checks. All four files pass: 164 passed. |
The sweep-test-coverage-state.csv is repo-wide bookkeeping that several sweeps mutate in parallel. It conflicts on GitHub because the merge=union driver does not run there. Reverting it to main's version makes this PR contain only the D8 degenerate-shape tests.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #2713.
Adds tests for degenerate raster shapes (1x1, 1xN, Nx1) to four D8 hydrology functions that had no single-pixel or strip coverage:
flow_accumulation_d8,flow_direction_d8,flow_length_d8, andhand_d8.flow_direction_d8: output is all-NaN at the input shape, since every cell is an edge cell.flow_accumulation_d8,flow_length_d8,hand_d8: output is finite at the input shape. The flow_length test covers bothdirectionmodes.Tests only, no source changes.
Backends: numpy. The degenerate-shape question is about kernel-boundary handling, and the numpy path is the reference. cupy, dask+numpy, and dask+cupy parity is already covered by existing tests in each of these files.
Test plan:
pyteston the four touched files passes on a CUDA host: 164 passed, including the 15 new cases. cupy and dask+cupy tests ran rather than skipped.